Update resource path to be prefixed with resource name #393
Conversation
knative-prow-robot
added
approved
Why: If task has multiple input resources defined without target path then all of them would be copied into /workspace folder and would overlap files. What: With this PR input resources will be placed under file path `/workspace/task_resource_name` . This will provide specific filepath for each resource and there should be no clash.
shashwathi
force-pushed
the
input-resource
branch
from
ba243ac to
dc68822
Compare
|
The following is the coverage report on pkg/.
|
There was a problem hiding this comment.
Thanks @shashwathi for the fix 👏
Can we just somehow make the document more clear and add a TODO or a new issue to make the copying of resources to PVC only happen when that resource is needed by a following task
| - Copy step includes resource being copied to PVC to path | ||
| `/pvc/task_name/resource_name` from `/workspace/random-space/` if resource | ||
| is declared in input resource like below example. | ||
| - If resource is declared both in input and output for task then copy step includes resource being copied to PVC to path `/pvc/task_name/resource_name` from `/workspace/random-space/` if input resource has custom target directory(`random-space`) declared like the following example. |
There was a problem hiding this comment.
In the different examples here, my understanding is that the resource will be copied to the PVC in the path /pvc/task_name/resource_name in all of the cases, if this is correct why do we need 3 examples?
There was a problem hiding this comment.
We need all the examples to show that source path when copied to PVC can change depending on
- resource only in output
- resource in input and output (no target path)
- resource in input and output( with target path)
|
@pivotal-nader-ziada : #398 for optimization for copy step to PVC |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shashwathi, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
Thanks for fixing this @shashwathi ! |
…ame" - In PR tektoncd#393 resource was placed under path "/workspace/resource_name", there is a use case where a resource can be used by a task multiple times as source with duplicate names. - Add more unit test to have more coverage for this use case. - Enable helm deploy task in this PR (tektoncd#393 PR disabled it as there is cyclic dependency)
…ame" - In PR tektoncd#393 resource was placed under path "/workspace/resource_name", there is a use case where a resource can be used by a task multiple times as source with duplicate names. - update storage resource(gcs) to have new namespace path as well - Add more unit test to have more coverage for this use case. - Enable helm deploy task in this PR (tektoncd#393 PR disabled it as there is cyclic dependency)
…ame" - In PR #393 resource was placed under path "/workspace/resource_name", there is a use case where a resource can be used by a task multiple times as source with duplicate names. - update storage resource(gcs) to have new namespace path as well - Add more unit test to have more coverage for this use case. - Enable helm deploy task in this PR (#393 PR disabled it as there is cyclic dependency)
Current implementation fetches resources into path
/workspace. If task has multiple input resources defined without target path, then resource files can overlap.With this PR input resources will be places under file path
/workspace/task_resource_name. This will provide specific filepath for each resource and there should be no clash as input resources with same names are not allowed.Note : I have disabled HelmDeployRun e2e test as that requires follow up PR. Test relies on master branch having changes already. It really makes me think there should separate repo with example chart to deploy because we cannot commit change and updated test in same PR.
cc @pivotal-nader-ziada @bobcatfish